Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Save WireGuard obfuscation settings choice and move DNS settings to a custom view #5433

Merged

Conversation

buggmagnet
Copy link
Contributor

@buggmagnet buggmagnet commented Nov 14, 2023

This PR is mostly accomplishing 2 goals:

  • Wiring up the WireGuard obfuscation settings to be saved in the tunnel settings
  • Separating the WireGuard settings and the DNS settings into 2 different screens

The PR looks big, but most of the code is just being moved around.
The aim was to declutter PreferencesDataSource, parts of it were moved to CustomDNSDataSource

Open Questions

  • We are thinking of adding a "WireGuard settings" title below the cell that goes to the DNS settings for better visual separation of sections

Here's what it looked like before the changes
Screenshot 2023-11-14 at 11 25 04

Here's what it looks like after the changes

Screenshot 2023-11-14 at 11 25 36

Screenshot 2023-11-14 at 11 25 49


This change is Reviewable

Copy link

linear bot commented Nov 14, 2023

Copy link
Contributor

@rablador rablador left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 13 of 13 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @buggmagnet)


ios/MullvadVPN/View controllers/Preferences/CustomDNSDataSource.swift line 247 at r1 (raw file):

    // MARK: - UITableViewDelegate

    func tableView(_ tableView: UITableView, shouldHighlightRowAt indexPath: IndexPath) -> Bool {

Do we need this?


ios/MullvadVPN/View controllers/Preferences/CustomDNSDataSource.swift line 358 at r1 (raw file):

    private func updateSnapshot(animated: Bool = false, completion: (() -> Void)? = nil) {
        var newSnapshot = NSDiffableDataSourceSnapshot<Section, Item>()
        let oldSnapshot = snapshot()

Diffdatasource figures things out this internally and does the diffing by itself, so I think we should remove oldSnapshot and get the identifiers from Item enum directly instead.

Copy link
Contributor Author

@buggmagnet buggmagnet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @rablador)


ios/MullvadVPN/View controllers/Preferences/CustomDNSDataSource.swift line 247 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Do we need this?

highlightedRows.mov
This is what happens if we don't have it (notice how the rows change colors as I tap on them)


ios/MullvadVPN/View controllers/Preferences/CustomDNSDataSource.swift line 358 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Diffdatasource figures things out this internally and does the diffing by itself, so I think we should remove oldSnapshot and get the identifiers from Item enum directly instead.

I tried to implement that, but it didn't work.

My understanding is that we still need the old snapshot when we collapse sections, and when we toggle editing mode to avoid the section collapsing if was expanded prior to that I think.

Or maybe I misunderstood your suggestion, how would it look ?

Here's what I tried

var newSnapshot = NSDiffableDataSourceSnapshot<Section, Item>()
newSnapshot.appendSections(Section.allCases)
newSnapshot.appendItems(Item.contentBlockers, toSection: .contentBlockers)

Copy link
Contributor

@rablador rablador left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @buggmagnet)


ios/MullvadVPN/View controllers/Preferences/CustomDNSDataSource.swift line 358 at r1 (raw file):

Previously, buggmagnet wrote…

I tried to implement that, but it didn't work.

My understanding is that we still need the old snapshot when we collapse sections, and when we toggle editing mode to avoid the section collapsing if was expanded prior to that I think.

Or maybe I misunderstood your suggestion, how would it look ?

Here's what I tried

var newSnapshot = NSDiffableDataSourceSnapshot<Section, Item>()
newSnapshot.appendSections(Section.allCases)
newSnapshot.appendItems(Item.contentBlockers, toSection: .contentBlockers)

Ah yes, entering edit mode makes things more complicated. Ok, let's leave it as it is.

@buggmagnet buggmagnet self-assigned this Nov 16, 2023
@buggmagnet buggmagnet added the iOS Issues related to iOS label Nov 16, 2023
@buggmagnet buggmagnet force-pushed the bind-wireguard-obfuscation-settings-to-tunnelsettings-ios-387 branch from 05da1a8 to 40e289e Compare November 20, 2023 13:37
Copy link
Contributor

@rablador rablador left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@buggmagnet buggmagnet force-pushed the bind-wireguard-obfuscation-settings-to-tunnelsettings-ios-387 branch 2 times, most recently from 8c1a466 to 3799a04 Compare November 22, 2023 10:35
@buggmagnet buggmagnet force-pushed the bind-wireguard-obfuscation-settings-to-tunnelsettings-ios-387 branch from 3799a04 to bf20c3b Compare November 22, 2023 15:31
@buggmagnet buggmagnet merged commit 7df5031 into main Nov 22, 2023
4 checks passed
@buggmagnet buggmagnet deleted the bind-wireguard-obfuscation-settings-to-tunnelsettings-ios-387 branch November 22, 2023 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iOS Issues related to iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants